Replace map with vector to remove key-pair restriction for System data #894
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors System.others_ from a map to a vector structure to support storing multiple properties (e.g., number concentration and density) for the same aerosol mode. Additionally, it introduces aerosol scoping functionality in ChemicalReactionBuilder to automatically prefix species names with mode and phase identifiers.
Key changes:
- Changed
System.others_fromstd::unordered_map<std::string, std::string>tostd::vector<std::string> - Added
JoinStringsutility function for concatenating dot-separated names - Introduced
SetAerosolScopemethod inChemicalReactionBuilderwith mutual exclusivity enforcement
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| include/micm/system/system.hpp | Changed others_ from map to vector, updated UniqueNames() and StateSize() methods, added JoinStrings usage |
| include/micm/util/utils.hpp | Added new JoinStrings utility function for joining strings with dot separators |
| include/micm/process/chemical_reaction_builder.hpp | Added SetAerosolScope method with scoping logic, updated SetReactants and SetProducts to support scoping |
| include/micm/process/process_error.hpp | Added InvalidConfiguration error code and updated error message ordering |
| include/micm/util/error.hpp | Added MICM_PROCESS_ERROR_CODE_INVALID_CONFIGURATION constant |
| test/unit/system/test_system.cpp | Updated test to use vector-based API with emplace_back |
| test/unit/solver/test_state.cpp | Updated tests to use vector-based API with push_back |
| test/unit/process/test_process_configuration.cpp | Added comprehensive tests for aerosol scoping functionality |
| test/unit/process/CMakeLists.txt | Registered new aerosol scope test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #894 +/- ##
==========================================
+ Coverage 94.64% 94.66% +0.02%
==========================================
Files 65 66 +1
Lines 3231 3245 +14
==========================================
+ Hits 3058 3072 +14
Misses 173 173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| namespace micm | ||
| { | ||
| inline std::string JoinStrings(const std::vector<std::string>& names) |
There was a problem hiding this comment.
can we do this with std::fmt?
There was a problem hiding this comment.
I updated it but It looks like std::format_to isn’t available with the GCC/libstdc++ versions we currently support. Should we revert the change or trying updating a newer GCC?
There was a problem hiding this comment.
oh, I guess revert the change. Maybe sometime later this year we can adopt support for std::fmt
Systems.others_is intended to store the data that are not species, such as mode specific number concentration. However, since it is implemented as a map, for example, mode-specific density can not be stored alongside number concentration.This PR replaces the map with a vector, removing key-pair restriction.